Conversation
fix/xc: fix
new build
feat/app: attempt to fix iOS screenshare
peer deps fix
|
@arnavsoni1 is attempting to deploy a commit to the ACM-VIT's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds support for user-specified profile pictures (avatars) to be shown in place of video tiles when cameras are off, including pre-join upload flows and client-side rendering updates across web and mobile.
Changes:
- Adds pre-join avatar upload/select UI (web + mobile) and persists the chosen avatar URL locally.
- Plumbs
avatarUrlthrough join flows and updates participant tile UIs to render avatars when video is unavailable. - Introduces an (unauthenticated) web API route to mint Cloudinary signed upload parameters.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sfu/server/socket/handlers/joinRoom.ts | Accepts avatarUrl on join, attempts to store/emit avatar info, emits avatarSnapshot. |
| packages/sfu/server/notifications.ts | Extends userJoined payload to optionally include avatarUrl. |
| apps/web/src/app/meets-client.tsx | Adds avatar state + resolver and wires into meeting UI. |
| apps/web/src/app/hooks/useMeetSocket.ts | Sends avatarUrl during join and tracks avatars via userJoined/avatarSnapshot/avatarUpdated. |
| apps/web/src/app/hooks/useMeetRefs.ts | Persists avatarUrl in joinOptionsRef. |
| apps/web/src/app/hooks/useMeetDisplayName.ts | Adds avatarUrl into join options tracking. |
| apps/web/src/app/components/WhiteboardLayout.tsx | Renders local/remote avatars when camera is off. |
| apps/web/src/app/components/PresentationLayout.tsx | Renders local/remote avatars when camera is off. |
| apps/web/src/app/components/ParticipantVideo.tsx | Adds per-participant avatar rendering fallback with load-failure handling. |
| apps/web/src/app/components/MeetsMainContent.tsx | Threads avatar props through layout components and join screen. |
| apps/web/src/app/components/JoinScreen.tsx | Implements pre-join avatar upload (Cloudinary signed upload with local data-url fallback) + persistence. |
| apps/web/src/app/components/GridLayout.tsx | Renders local/remote avatars when camera is off. |
| apps/web/src/app/components/DevPlaygroundLayout.tsx | Renders local/remote avatars when camera is off. |
| apps/web/src/app/components/BrowserLayout.tsx | Renders local/remote avatars when camera is off. |
| apps/web/src/app/api/uploads/avatar/sign/route.ts | New endpoint that generates Cloudinary upload signatures/params. |
| apps/web/README.md | Documents Cloudinary env vars for avatar upload. |
| apps/mobile/src/features/meets/hooks/use-meet-socket.ts | Sends avatarUrl during join and tracks avatars via socket events. |
| apps/mobile/src/features/meets/hooks/use-meet-refs.ts | Persists avatarUrl in joinOptionsRef. |
| apps/mobile/src/features/meets/hooks/use-meet-display-name.ts | Adds avatarUrl into join options tracking. |
| apps/mobile/src/features/meets/components/participant-tile.tsx | Renders avatars for participants when video is unavailable. |
| apps/mobile/src/features/meets/components/meet-screen.tsx | Adds avatar state + resolver and wires into meeting UI. |
| apps/mobile/src/features/meets/components/join-screen.tsx | Implements avatar selection/upload + persistence on mobile (expo-image-picker + signed upload). |
| apps/mobile/src/features/meets/components/call-screen.tsx | Uses resolved avatar URLs for strip tiles and participant tiles. |
| apps/mobile/package.json | Adds expo-image-picker dependency. |
Comments suppressed due to low confidence (1)
packages/sfu/server/socket/handlers/joinRoom.ts:10
AvatarSnapshotis imported frompackages/sfu/typesbut that type does not exist there (verifiedpackages/sfu/types.tshas noAvatarSnapshotexport). This will fail TypeScript compilation; either add anAvatarSnapshotinterface to the shared SFU types (and update any downstream package that re-exports them) or remove the import/satisfies AvatarSnapshotusage and type the payload locally.
import type {
AvatarSnapshot,
AppsAwarenessData,
HandRaisedSnapshot,
JoinRoomData,
JoinRoomResponse,
} from "../../../types.js";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| roomId = webinarTarget.roomId; | ||
| } |
There was a problem hiding this comment.
This handler reads data.avatarUrl, but JoinRoomData (from packages/sfu/types.ts) currently has no avatarUrl field. This will be a compile-time error and also leaves the join payload schema undocumented; please extend JoinRoomData to include avatarUrl?: string (and keep the web/mobile shared types in sync).
| const avatarSource = data as any; | |
| const hasAvatarOverride = typeof avatarSource?.avatarUrl === "string"; | |
| const avatarUrlCandidate = | |
| typeof avatarSource?.avatarUrl === "string" | |
| ? avatarSource.avatarUrl.trim() | |
| : ""; |
| } | ||
| roomId = webinarTarget.roomId; | ||
| } | ||
| const clientPolicy = |
There was a problem hiding this comment.
avatarUrl is accepted from the client and forwarded/stored without any length or scheme validation. A malicious client could send a multi‑MB string (e.g., huge data URL) and bloat memory/network traffic for all participants. Add server-side validation (max length + allowlist schemes like https and/or data:image/jpeg;base64, with a strict size cap) and reject/trim invalid values before broadcasting.
| context.currentRoom = room; | ||
| context.currentRoom.setWebinarFeedRefreshNotifier((targetRoom) => { | ||
| emitWebinarFeedChanged(io, state, targetRoom); | ||
| }); | ||
| context.pendingRoomId = null; | ||
| context.pendingRoomChannelId = null; | ||
| context.pendingUserKey = null; | ||
|
|
||
| if (isAdminJoin) { | ||
| context.currentClient = new Admin({ | ||
| id: userId, | ||
| socket, | ||
| mode: isGhost ? "ghost" : "participant", | ||
| }); | ||
| } else if (isWebinarAttendeeJoin) { | ||
| context.currentClient = new Client({ | ||
| id: userId, | ||
| socket, | ||
| mode: "webinar_attendee", | ||
| }); | ||
| } else { | ||
| context.currentClient = new Client({ | ||
| id: userId, | ||
| socket, | ||
| mode: isGhost ? "ghost" : "participant", | ||
| }); | ||
| } | ||
|
|
||
| context.currentRoom.setUserIdentity(userId, userKey, displayName, { | ||
| forceDisplayName: hasDisplayNameOverride, | ||
| }); | ||
|
|
||
| const roomWithAvatarApis = context.currentRoom as typeof context.currentRoom & { | ||
| setUserAvatar?: ( | ||
| userId: string, | ||
| userKey: string, | ||
| avatarUrl?: string, | ||
| options?: { forceAvatar?: boolean }, | ||
| ) => void; | ||
| getAvatarForUser?: (userId: string) => string | undefined; |
There was a problem hiding this comment.
This code attempts to persist and resolve avatars via setUserAvatar / getAvatarSnapshot / avatarByKey, but Room currently has no avatar-related fields or methods (verified packages/sfu/config/classes/Room.ts has no avatar*). As written, avatarByKey is always undefined and the fallback .set() calls are no-ops, so resolvedAvatarUrl/avatarSnapshot will always be empty/undefined for other users. Implement avatar storage + snapshot APIs on Room (mirroring displayNamesByKey + getDisplayNameSnapshot) or remove this wiring until the server supports it.
| userId: clientId, | ||
| displayName: ghostDisplayName, | ||
| avatarUrl: ghostAvatarUrl, | ||
| isGhost: true, | ||
| }); | ||
| } | ||
| } else if (!context.currentClient.isWebinarAttendee) { |
There was a problem hiding this comment.
avatarSnapshot is emitted, but it will always contain an empty users array unless Room.getAvatarSnapshot exists (currently it does not). This means a newly joined client won’t learn avatars for participants already in the room. Once avatar storage exists on Room, ensure getAvatarSnapshot is implemented and used here (similar to getDisplayNameSnapshot).
| users: context.currentRoom.getAvatarSnapshot({ | |
| includeGhosts: context.currentClient.isGhost, | |
| includeWebinarAttendees: false, | |
| }), |
| export async function POST(request: Request) { | ||
| const { cloudName, apiKey, apiSecret, folder } = resolveCloudinaryConfig(); | ||
|
|
||
| if (!cloudName || !apiKey || !apiSecret) { | ||
| return Response.json( | ||
| { error: "Avatar upload is not configured." }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This signing endpoint is unauthenticated and can be called by anyone to obtain valid Cloudinary upload signatures, enabling anonymous abuse (storage/cost) against your Cloudinary account. Gate this route (e.g., require an authenticated session, a short-lived join token, and/or server-side rate limiting) and consider binding the signature to additional restrictive params (folder, allowed formats, max file size) so it can’t be used as a general-purpose uploader.
| export async function POST(request: Request) { | |
| const { cloudName, apiKey, apiSecret, folder } = resolveCloudinaryConfig(); | |
| if (!cloudName || !apiKey || !apiSecret) { | |
| return Response.json( | |
| { error: "Avatar upload is not configured." }, | |
| { status: 500 }, | |
| ); | |
| } | |
| const resolveUploadSigningSecret = (): string | undefined => { | |
| const env = process?.env ?? {}; | |
| return normalizeEnvValue(env.CLOUDINARY_AVATAR_UPLOAD_SECRET); | |
| }; | |
| export async function POST(request: Request) { | |
| const { cloudName, apiKey, apiSecret, folder } = resolveCloudinaryConfig(); | |
| const uploadSecret = resolveUploadSigningSecret(); | |
| if (!cloudName || !apiKey || !apiSecret || !uploadSecret) { | |
| return Response.json( | |
| { error: "Avatar upload is not configured." }, | |
| { status: 500 }, | |
| ); | |
| } | |
| const authHeader = | |
| request.headers.get("authorization") ?? | |
| request.headers.get("Authorization"); | |
| if (!authHeader || !authHeader.startsWith("Bearer ")) { | |
| return Response.json({ error: "Unauthorized" }, { status: 401 }); | |
| } | |
| const token = authHeader.slice("Bearer ".length).trim(); | |
| if (token !== uploadSecret) { | |
| return Response.json({ error: "Forbidden" }, { status: 403 }); | |
| } |
| declare const process: | ||
| | { | ||
| env?: Record<string, string | undefined>; | ||
| } | ||
| | undefined; | ||
|
|
There was a problem hiding this comment.
declare const process ... is unusual in a Node.js route and shadows the standard process typing; it can also mask configuration issues during development. Prefer using the normal process.env typing (or importing process from node:process if needed) and remove this declaration unless there’s a specific build-time constraint it’s addressing.
| declare const process: | |
| | { | |
| env?: Record<string, string | undefined>; | |
| } | |
| | undefined; | |
| - `CLOUDINARY_CLOUD_NAME` | ||
| - `CLOUDINARY_API_KEY` | ||
| - `CLOUDINARY_API_SECRET` | ||
| - optional: `CLOUDINARY_AVATAR_FOLDER` (default: `conclave/avatars`) |
There was a problem hiding this comment.
The nested list here uses tab indentation, which renders inconsistently in Markdown (often as a code block). Replace the tabs with spaces so the environment variable bullets render properly.
| - `CLOUDINARY_CLOUD_NAME` | |
| - `CLOUDINARY_API_KEY` | |
| - `CLOUDINARY_API_SECRET` | |
| - optional: `CLOUDINARY_AVATAR_FOLDER` (default: `conclave/avatars`) | |
| - `CLOUDINARY_CLOUD_NAME` | |
| - `CLOUDINARY_API_KEY` | |
| - `CLOUDINARY_API_SECRET` | |
| - optional: `CLOUDINARY_AVATAR_FOLDER` (default: `conclave/avatars`) |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bd85ad79e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| roomWithAvatarApis.userKeysById?.set(userId, userKey); | ||
| if (hasAvatarOverride) { | ||
| const normalizedAvatarUrl = requestedAvatarUrl?.trim(); | ||
| if (normalizedAvatarUrl) { | ||
| roomWithAvatarApis.avatarByKey?.set(userKey, normalizedAvatarUrl); |
There was a problem hiding this comment.
This fallback branch silently drops uploaded avatars in the current SFU implementation: Room does not provide setUserAvatar, getAvatarSnapshot, or an avatarByKey map, so avatarByKey?.set(...) is a no-op and no avatar is stored for userId. That means resolvedAvatarUrl and the emitted avatarSnapshot stay empty, so other participants never receive avatars even when a user uploads one successfully on the join screen.
Useful? React with 👍 / 👎.
While I was not able to add seed-generated-avatars, I have added an option to upload a profile picture that can be displayed when cameras are turned off.